feat(micropub): add proposed contacts extension - #368
Conversation
davidwkeith
left a comment
There was a problem hiding this comment.
Automated code review.
CONTRIBUTING.md conformance: PASS. Title/scope, PR template headings, changeset (.changeset/micropub-contacts.md), and spec update are all in order. The unticked "full CI gate" and "catalog.json" boxes carry one-line reasons, which matches the "leave unchecked with a reason" rule — not a violation, but please run the full pnpm lint && pnpm format:check && pnpm typecheck && pnpm build && pnpm test before taking this out of draft.
Code issues found:
-
Should-fix — silent data loss on multipart contact create. In
handleAction()(packages/micropub/src/handler.ts), theisContactRequestbranch returns before thefoldUploadedMedia/pendingFilesblock runs. A?q=contactcreate sent asmultipart/form-datawith aphotofile returns 201 but the file is never uploaded to R2 or folded into properties — silently dropped. Given this repo's "fail loudly, no silent degradation" convention, please either fold uploads for contacts too or explicitly reject multipart contact requests. -
Should-fix — missing scope-enforcement test. The new
describe("@dwk/micropub proposed contacts")tests never check that a token without the right scope (e.g.create-only attemptingaction=delete) getsinsufficient_scope/403.scopesForActionis reused unmodified from the post path so it's likely correct, but the spec explicitly calls out per-action scoping for contacts and it's currently unverified by any test. -
Worth confirming — contacts readable by any authenticated token.
handleQuery()callsauthorize(..., [], ...), so any valid token (evenmedia-scoped) can read the address book. This mirrors existingq=sourcebehavior, but since the new spec text calls contacts "private owner data," please confirm that's intentional rather than inherited by accident. -
Nit —
contactView()collapses single-value properties to a bare scalar, diverging from the array-always shapesourceView()and the rest of the mf2 JSON surface use. Called out in the added spec text, but flagging since it could surprise clients sharing mf2-parsing code acrossq=source/q=contact. -
Nit — the "URL already in use" conflict message is identical for a genuine URL-identity conflict and a (practically impossible) UUID PK collision; harmless but slightly misleading.
No injection/auth-bypass issues found — parameterized D1 queries throughout, DPoP/scope gating structurally correct, no KV usage.
Generated by Claude Code
# Conflicts: # packages/micropub/README.md # packages/micropub/src/config.ts # packages/micropub/src/index.ts # spec/packages/micropub.md
# Conflicts: # spec/packages/micropub.md
Summary
q=contact) extension.and DPoP/scope protection.
Packages affected
Checklist
spec/packages/and updated them ifbehaviour changed
src/*.test.ts)pnpm lint && pnpm format:check && pnpm typecheck && pnpm build && pnpm test— targeted Micropub typecheck and 124 tests passed; full gate not runpnpm changeset) if this touches a publishablepackage
catalog.json/conformance/status.jsonif this adds a newmountable worker or changes conformance status — not applicable, no new worker